refactor(esplora): clear remaining panic paths #2053
Conversation
crates/esplora/src/async_ext.rs
Outdated
| let last_index = | ||
| last_index.ok_or_else(|| Box::new(esplora_client::Error::InvalidResponse))?; |
There was a problem hiding this comment.
One suggestion is to leave last_index as an Option and wrap stop_gap in Some(..) when doing the comparison.
But there's a much better way to handle this, which is to introduce an unused_count variable. Now when processing handles, if txs.is_empty() we increment the unused count, else update the last active index (and reset the unused count). Finally, break once the unused count reaches the gap limit. AFAICT there'd be no reason to keep track of the last_index scanned.
There was a problem hiding this comment.
Took your suggestion and worked it in; didn't want to push it to this branch yet if I had the wrong idea, but here is the commit reez@35538e2 on a branch that builds on this branch.
I felt like I had to keep two small guardrails from the old logic:
processed_any(still surface the “no handles ran” error)gap_limit = stop_gap.max(1)(keeps old “stop_gap==0 still means one empty derivation” behavior)
There was a problem hiding this comment.
I don't think it's an error if handles turns up empty - we need a way to break from the loop once all of the keychain-spks are consumed.
There was a problem hiding this comment.
Maybe stop_gap = stop_gap.max(parallel_requests), since we check at least 1 spk-index for every request in parallel_requests.
There was a problem hiding this comment.
Super helpful thanks mammal, I made the below changes to that my side branch esplora+mammal that I can eventually fold back into this esplora branch if I'm on the right track
I don't think it's an error if handles turns up empty - we need a way to break from the loop once all of the keychain-spks are consumed.
Good call. I made this change 36073d0 based on that
Maybe stop_gap = stop_gap.max(parallel_requests), since we check at least 1 spk-index for every request in parallel_requests.
Good catch, made another change 590edcf based on this
There was a problem hiding this comment.
pulled these changes into this pr now
crates/esplora/src/blocking_ext.rs
Outdated
| let (index, txs, evicted) = handle.join().expect("thread must not panic")?; | ||
| let handle_result = handle | ||
| .join() | ||
| .map_err(|_| Box::new(esplora_client::Error::InvalidResponse))?; |
There was a problem hiding this comment.
Because you're dropping the error it won't be very informative to propagate it upward. Instead this module can define its own Error type to handle errors that aren't covered by esplora_client::Error.
There was a problem hiding this comment.
Tried to address this with new commit 9ea0dd0, trying to do the most simple thing I could think of that addresses what you mentioned
reez
changed the title
| #[derive(Debug)] | ||
| pub enum Error { | ||
| Client(esplora_client::Error), | ||
| ThreadPanic(Option<String>), |
There was a problem hiding this comment.
This seems over-engineered. Thread panics are highly unlikely because:
- No explicit panics in the thread - all errors are propagated.
- We only call
esplora_clientmethods - if the method panics, it's a bug in theesplora_clientlibrary and should be addressed in the upstream crate.
ThreadPanic is really only protection against bugs in the dependency chain (should be addressed upstream) or extreme memory exhaustion (which would crash the application anyway).
| #[test] | ||
| fn ensure_last_index_none_returns_error() { | ||
| let last_index: Option<u32> = None; | ||
| let err = last_index | ||
| .ok_or_else(|| Box::new(esplora_client::Error::InvalidResponse)) | ||
| .unwrap_err(); | ||
| assert!(matches!(*err, esplora_client::Error::InvalidResponse)); | ||
| } |
There was a problem hiding this comment.
I don't think this is needed. If this fails, it's a problem with rust.
| if handles.is_empty() { | ||
| if !processed_any { | ||
| return Err(esplora_client::Error::InvalidResponse.into()); | ||
| } |
There was a problem hiding this comment.
Looks like you are trying to catch a empty-input? You'll need a justification on why an empty input should result in an error. My understanding is that the caller may create a FullScanRequest with an empty keychain and if they do, it's not problematic at all and we should just return nothing (which looks like what the original code is doing).
| #[test] | ||
| fn thread_join_panic_maps_to_error() { | ||
| let handle = std::thread::spawn(|| -> Result<(), Error> { | ||
| panic!("expected panic for test coverage"); | ||
| }); | ||
|
|
||
| let res = (|| -> Result<(), Error> { | ||
| let handle_result = handle | ||
| .join() | ||
| .map_err(Error::from_thread_panic)?; | ||
| handle_result | ||
| })(); | ||
|
|
||
| assert!(matches!( | ||
| res.unwrap_err(), | ||
| Error::ThreadPanic(_) | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn ensure_last_index_none_returns_error() { | ||
| let last_index: Option<u32> = None; | ||
| let err = last_index | ||
| .ok_or_else(|| Error::from(esplora_client::Error::InvalidResponse)) | ||
| .unwrap_err(); | ||
| assert!(matches!(err, Error::Client(esplora_client::Error::InvalidResponse))); | ||
| } | ||
|
|
There was a problem hiding this comment.
No need to test rust for bugs in our repo.
3 participants
Description
This PR clears remaining panic paths from bdk_esplora, replacing them with proper error handling.
Attempting to address bitcoindevkit/bdk_wallet#30 for Esplora
Notes to the reviewers
Open to any and all feedback on this.
Other PR's made along these lines:
unwrap()s andexpect()s #1981 (electrum)chain_updateerrors if no point of connection #1971 (esplora)Changelog notice
bdk_esplora.rs.Checklists
All Submissions:
New Features:
Bugfixes: